build: Enable TypeScript type checking#7680
Conversation
| var constants = require('./constants'); | ||
| var overrideAll = require('../../plot_api/edit_types').overrideAll; | ||
| var sortObjectKeys = require('../../lib/sort_object_keys'); | ||
| var sortObjectKeys = require('../../lib/sort_object_keys').default; |
There was a problem hiding this comment.
@camdecoster Is it worth considering moving away from default exports entirely as part of this transition?
There was a problem hiding this comment.
It only matters when one mixes CJS and ESM. That said, I don't think that we can avoid that for a long time. I suppose we could make just pick a direction to go and standardize. I could go either way, but the esbuild docs make it clear that they don't like default exports.
| fillcolor?: string; | ||
| line?: Partial<Line>; | ||
| marker?: Partial<Marker>; | ||
| mode?: 'lines' | 'markers' | 'lines+markers' | 'none' | 'text' | 'lines+text' | 'markers+text' | 'lines+markers+text'; |
There was a problem hiding this comment.
Does TypeScript have any support for 'flag list'-type string values such as this, where the string may consist of any number of a fixed set of values joined by a delimiter? I suppose not as it's fairly custom.
Otherwise could we use a custom type or a custom function to generate these lists of allowed values based on the flags?
There was a problem hiding this comment.
Yes, it's called a union type. What's shown on 133 is an example of that (though it's only used on that line). If we needed that type elsewhere, we could define it separately and reference it within the ScatterTrace type like this:
type Mode = 'lines' | 'markers' | 'lines+markers' | 'none' | 'text' | 'lines+text' | 'markers+text' | 'lines+markers+text';
export interface ScatterTrace extends TraceBase {
...,
mode: Mode;
...,
}There was a problem hiding this comment.
Yeah I guess I'm imagining something like a helper function which looks like flagList(flaglistVals, otherVals) such that
flaglistVals(['lines', 'markers', 'text'], ['none'])
returns
'lines' | 'markers' | 'lines+markers' | 'none' | 'text' | 'lines+text' | 'markers+text' | 'lines+markers+text';`
| * Use specific trace types when available | ||
| */ | ||
| export interface GenericTrace extends TraceBase { | ||
| x?: any[]; |
There was a problem hiding this comment.
I'm not sure there are any traces where x/y/z are allowed to be a type other than number[] | string[] but I could be wrong about that.
There was a problem hiding this comment.
That's why it's permissive here. Once we become sure, we can change this as you suggest.
There was a problem hiding this comment.
In this case maybe it would be easier to start stricter and loosen if needed?
|
@camdecoster Can you add a type-check step to the CI? |
| yaxis?: Partial<LayoutAxis>; | ||
|
|
||
| // Multiple axes support (xaxis2, yaxis3, etc.) | ||
| [key: string]: any; |
There was a problem hiding this comment.
Can we tighten this layout spec here to match the plot schema?
|
@camdecoster Some initial thoughts on organization of the types: For types corresponding directly to the schema:
|
codeCraft-Ritik
left a comment
There was a problem hiding this comment.
Great work! The implementation is clean and easy to understand
2439dad to
de168ca
Compare
| * Common properties shared by all trace types — convenience interface for | ||
| * gradual migration of internal code that doesn't need the full PlotData shape. | ||
| */ | ||
| export interface TraceBase { |
There was a problem hiding this comment.
What's the relationship between the TraceBase and PlotData types?
| _length?: number; | ||
| _module?: any; | ||
| index?: number; | ||
| [key: string]: any; |
There was a problem hiding this comment.
Is there any logic behind which keys are explicitly listed here?
| } | ||
|
|
||
| // --------------------------------------------------------------------------- | ||
| // LayoutAxis (extends Axis for cartesian subplots) |
There was a problem hiding this comment.
small nit, but I'm not sure this comment is accurate since some of these properties seem to exist for non-cartesian axes as well
| // Mapbox types | ||
| // --------------------------------------------------------------------------- | ||
|
|
||
| export interface MapboxCenter { |
There was a problem hiding this comment.
All of these Mapbox* types can be changed to Map* right? AFAIK the API is exactly the same.
| xaxis6: Partial<LayoutAxis>; | ||
| xaxis7: Partial<LayoutAxis>; | ||
| xaxis8: Partial<LayoutAxis>; | ||
| xaxis9: Partial<LayoutAxis>; |
There was a problem hiding this comment.
hm I realize this is taken from DefinitelyTyped but what about plots with more than 9 x/y axes? I don't think there's any hard upper limit
| hoversubplots: 'single' | 'overlaying' | 'axis'; | ||
| calendar: Calendar; | ||
|
|
||
| // Dotted property paths for Plotly.relayout convenience |
| _modeBar?: any; | ||
| _pushmargin?: { [key: string]: any }; | ||
|
|
||
| [key: string]: any; |
There was a problem hiding this comment.
same question here, is there any reasoning behind which keys are called out explicitly?
| /** | ||
| * Template figure type | ||
| */ | ||
| export interface TemplateFigure { |
There was a problem hiding this comment.
What's the difference between Template and TemplateFigure?
There was a problem hiding this comment.
Shouldn't this be generated from src/components/colorbar/attributes.js?
There was a problem hiding this comment.
Doesn't this file duplicate the info in src/traces/box/attributes.js to some extent?
|
|
||
| | User-facing | Internal | Where defined | | ||
| |---|---|---| | ||
| | `Layout` | `FullLayout` | `Layout` is generated; `FullLayout extends Layout` is hand-written | |
There was a problem hiding this comment.
`Layout` is generated; `FullLayout extends Layout` is hand-written
This isn't currently the case based on the contents of this PR though, right? Layout seems to be hand-written.
|
|
||
| ```ts | ||
| /** | ||
| * @generates ModeBar |
There was a problem hiding this comment.
Should probably add some guidelines for choosing this canonical public name, since it may not always be obvious.
|
|
||
| ### 6. Replace the hand-written type in `src/types/` | ||
|
|
||
| Find the corresponding hand-written type (likely in |
There was a problem hiding this comment.
Will it always be obvious which hand-written type corresponds to the attributes file? How do you determine for sure whether one already exists or not?
| If the hand-written type was richer than the schema (e.g. used a narrowed | ||
| union where the schema says `string`), document the gap in a comment or | ||
| file an issue. Do not silently lose ergonomics — either improve the schema | ||
| (add `values: [...]`) or layer a hand-written refinement on top. |
There was a problem hiding this comment.
or layer a hand-written refinement on top
I'm not sure there's really a clean way to do this using the proposed architecture
Description
Enable TypeScript type checking.
Closes #7678.
Changes
Testing
npm run buildlocally to check that everything is bundled properly by esbuildnpm run typecheckto perform a type check on the source code. There should be no errors:Notes
npm run typecheck. VS Code will also provide feedback as you're editing files.TS is not emitting files/types right now. esbuild handles converting the TS so the TS compiler won't need to do that. It would be valuable to have the compiler write the type definition files in the future.TS doesn't emit types, but there's a type generation step that uses TS to create types from attributes files.